-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(earn): prepare transactions and use on confirmation screen #6192
base: tomm/act-1389-0
Are you sure you want to change the base?
Conversation
src/earn/EarnConfirmationScreen.tsx
Outdated
}) | ||
// @ts-expect-error prepareTransactionsResult & swapTransaction are only set when mode === 'deposit' or 'swap-deposit' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand these two lines.
- Seems like a red flag to do a ts-ignore here. if the return values of the different modes are that different then should they maybe be changed at the prepare transaction level?
- swapTransaction doesn't even seem to be used in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ts-expect-error
is temporary; I'll remove it before moving the PR out of draft and resolving the type issues. While swapTransaction
isn't currently used on this page there's a bug with the bottom sheet implementation that will likely require swap-deposit
and deposit
to be handled from a confirmation screen.
src/earn/EarnConfirmationScreen.tsx
Outdated
pool, | ||
hooksApiUrl, | ||
// Mode === shortcutId, except for exit which is withdraw | ||
shortcutId: mode === 'exit' ? 'withdraw' : mode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So exit maps to 'prepareWithdrawAndClaimTransactions' which doesn't appear to have shortcutId as a parameter. So why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 98a8bd0.
src/earn/EarnEnterAmount.tsx
Outdated
@@ -144,6 +144,7 @@ function EarnEnterAmount({ route }: Props) { | |||
} | |||
|
|||
const { | |||
// @ts-expect-error prepareTransactionsResult & swapTransaction are only set when mode === 'deposit' or 'swap-deposit' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, the ts-expects-error is a big red flag. The underlying types should probably be changed or we just need separate functions
src/earn/prepareTransactions.ts
Outdated
|
||
// Prepare the claim transactions if there are rewards positions | ||
const claimTransactions = rewardsPositions | ||
? await Promise.all( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this checks if rewardsPositions
is falsy, but can it ever be falsy since its guarenteed to be an array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
Handles the preparation of all earn transactions via a shared hook that's used on both
EarnEnterAmount.tsx
&EarnConfirmationScreen.tsx
.Test plan
Related issues
Backwards compatibility
Yes
Network scalability
N/A